-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add comments to protobuf files about specifying ledgers #3398
Conversation
Requested the devx people for review, since they pointed this out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know enough about the gRPC details to really validate whether these statements are true. However, they make sense to me.
@mDuo13 gRPC and the JSON API use the same code underneath. If the comments are true for JSON, they are true for gRPC. Edit: they use the same code for tx and account_tx, but account_info is a different codepath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look reasonable to me but I'd wait for Amie's review as well since she's been closer to the metal here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one tiny nit but otherwise this is clear to me and agrees with the behavior I've observed WRT various configurations of ledger specifiers/ranges in gRPC calls from the SDKs. Thanks CJ!
@@ -23,7 +23,11 @@ message GetTransactionRequest { | |||
// if true, return data in binary format. defaults to false | |||
bool binary = 2; | |||
|
|||
// search only specified range. optional | |||
// If the transaction is not found, server will report whether the entire | |||
// specified range is searched. The value is contained in the error message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might change "is" to "was"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to was
e1351bd
@amiecorso @Stormtv can you approve this if it looks good? |
Specifying a ledger has a lot of gotchas. For
GetTransaction
, the ledger range is only used for error handling, and searched_all is part of the error message, as opposed to the protobuf response. ForGetAccountInfo
,SHORTCUT_VALIDATED
returns the most up to date data; however, usingSHORTCUT_VALIDATED
forGetAccountTransactionHistory
results in only transactions from the most recently validated ledger being returned. I added some comments to point out these nuances more clearly.This change is